Conversation
revu-bot
left a comment
There was a problem hiding this comment.
Summary
This PR fixes the critical bug where imports into non-partitioned tables were broken (issue #82). The implementation correctly adds conditional logic to use direct INSERT statements for standard tables while maintaining the stored procedure approach for partitioned tables.
Key Changes
- ✅ Adds proper branching logic based on
DESTINATION_TABLEconfiguration - ✅ Introduces column constants to prevent parameter mis-ordering
- ✅ Updates test expectations to reflect actual behavior
Issues Found
| Severity | Issue | Impact |
|---|---|---|
| CRITICAL | SQL injection vulnerability in dynamic table identifier | Security risk |
| IMPORTANT | Inconsistent SQL composition pattern | Maintainability & potential bugs |
| MINOR | Test comment accuracy | Documentation clarity |
The core fix is sound, but there's a critical security issue with dynamic table name handling that must be addressed before merging.
| // Direct INSERT for standard (non-partitioned) table | ||
| await sql` | ||
| INSERT INTO ${sql.id(DESTINATION_TABLE)} ( |
There was a problem hiding this comment.
[CRITICAL] SQL Injection Vulnerability in Dynamic Table Name
Why this matters:
DESTINATION_TABLEcomes from environment variables and is used directly insql.id()- If an attacker controls the environment variable, they could inject malicious SQL
- The
sql.id()function is designed for identifiers but doesn't validate the input is a safe table name
Recommended fix:
Validate that DESTINATION_TABLE matches expected table names before using it in SQL:
// At the top of the file or in config validation
const VALID_TABLE_NAMES = [MATOMO_TABLE_NAME, PARTITIONED_MATOMO_TABLE_NAME]
if (!VALID_TABLE_NAMES.includes(DESTINATION_TABLE)) {
throw new Error(`Invalid DESTINATION_TABLE: ${DESTINATION_TABLE}. Must be one of: ${VALID_TABLE_NAMES.join(', ')}`)
}Alternatively, use a safer approach by directly referencing the validated constant rather than passing through sql.id().
There was a problem hiding this comment.
If an attacker has control over our env variables we're in bigger trouble than that anyway
0c42576 to
9c0fdf1
Compare
vidy-br: fix-non-partitioned-tables
9c0fdf1 to
932db2b
Compare
|
|
🎉 This PR is included in version 2.3.14 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |



Fix #82